Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix for v1.5 UnnaturalCorpse event #445

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

CodeOptimist
Copy link

@CodeOptimist CodeOptimist commented Apr 18, 2024

I'm in the Discord as @CodeOptimist, ping me any time.
Tested and appears to work.

I tried to copy the existing style, I thought the Ldc_I4_1 Or used by DrawTrackerTickPatch beneath this was clever and that also transpiles a CellRect.Contains(), so I used the same though here we return false with Ldc_I4_0 And. I put this patch next to that patch because of the similarity, though perhaps move it if it belongs with other v1.5 patches.

I don't know much about this actual event, hopefully having the corpse move with one (or both) players watching doesn't ruin the whole gimmick, as doing things unwatched seems a little special/unique to this? Checking if it's unwatched by all clients sounds... complex.

Here's the unpatched vanilla methods:

UnnaturalCorpse

private bool IsOutsideView()
{
	return (!base.SpawnedOrAnyParentSpawned || !base.MapHeld.reservationManager.IsReservedByAnyoneOf(this, Faction.OfPlayer)) && (Find.CurrentMap != base.MapHeld || !Find.CameraDriver.CurrentViewRect.ExpandedBy(1).Contains(base.PositionHeld));
}

AnomalyUtility

public static bool IsValidUnseenCell(CellRect view, IntVec3 cell, Map map)
{
	return (map != Find.CurrentMap || !view.Contains(cell)) && !cell.Fogged(map) && cell.Walkable(map) && cell.GetFence(map) == null && cell.GetDoor(map) == null;
}

As my first PR: You're always free to aggressively reject/refactor/rewrite. 👍
Cheers. 🍻

@CodeOptimist
Copy link
Author

CodeOptimist commented Apr 18, 2024

Oh just for more info, I looked at patching it at a lower level but they didn't really seem relevant. More like UI stuff that didn't need patched?

image
(Obviously you can't see the uses of cellRect = from this screenshot, but I investigated them.)
Interesting to see that though CellRect has many methods, many of which return bools, I (think) I only see .Contains used for CurrentViewRect... which makes sense.

@Zetrith
Copy link
Member

Zetrith commented Apr 22, 2024

If you think about it, to ensure determinism, a method like IsValidUnseenCell can't ever return true in MP. The methods you patch either check the camera rect or Find.CurrentMap both of which are allowed to vary in multiplayer so these methods should just get a destructive prefix to always return false I think.

This will drastically affect the event though so I'll have to play around with it to decide how to handle it.

@CodeOptimist
Copy link
Author

CodeOptimist commented Apr 22, 2024

I forgot about Find.CurrentMap 😅

@CodeOptimist
Copy link
Author

CodeOptimist commented Apr 22, 2024

I think they're just combining tests of "validity" for the event but also "unseen" for spookiness. More like IsValidAndIsUnseen(). I think I can just patch out the map part also. They just don't want the player to "see" it teleport, but who cares. I think these are only used here. I can give it another go and report back. 👍

@SokyranTheDragon SokyranTheDragon added the anomaly Fix or bugs relating to Anomaly (Not 1.5) label Apr 28, 2024
@CodeOptimist
Copy link
Author

CodeOptimist commented May 5, 2024

Updated and tested in-game. Corpse teleports around now like it should.

Here's a diff of the before and after opcodes.
https://gist.github.com/CodeOptimist/454832f119239fbf3e8e4d4c6098a98e/revisions
(I was really hoping it would embed that link. I'm a little disappointed.)
(The diff is misaligned slightly; obviously patch adds lines 32 & 33, not 33 & 34.)

@CodeOptimist CodeOptimist force-pushed the unnatural-corpse branch 5 times, most recently from 9fdcb65 to 6e441f9 Compare May 6, 2024 14:46
@SokyranTheDragon
Copy link
Member

Thinking about this code again, I think only one patch would be enough, either to CellRect.Contains or Find.CurrentMap check. The condition in both cases could be simplified as (isTargetValid) && (map != Find.CurrentMap || !CellRect.Contains(target)).

We obviously cannot change the first part of the code, since this could likely introduce bugs (and same could be said about making a prefix to always return true). However, patching the second part would require changing at least one of the 2 conditions, as only 1 of them needs to be true (and this PR patches both).

Now, it is more of an observation I've made looking through some of the PRs that are both active or were merged. I'm wondering if we should keep both patches, or should we keep only the simpler of the 2?

@SokyranTheDragon SokyranTheDragon mentioned this pull request Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
anomaly Fix or bugs relating to Anomaly (Not 1.5)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants